[FS] Prevent duplicate Field Service customer assets during item synchronization - #11094
tomasevicst wants to merge 29 commits into
Conversation
github-actions
Bot
added
AL: Apps (W1)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The removed IgnoreServiceItem* tests, and the new ItemSynchronizationDisablesCustomerAssetConversion test, both call the helper procedure directly rather than exercising the actual dispatch path. Critically, the new 'Item-CRM Product' case added to OnBeforeTransferFieldData (which does DestinationRecordRef.SetTable(CRMProduct) / DisableCustomerAssetConversion(...) / DestinationRecordRef.GetTable(CRMProduct)) is never exercised by any test — no test drives an Item-to-CRMProduct transfer through the RecordRef dispatch to confirm the case is reached and the record is written back correctly. Add an integration-style test that triggers the Item-CRM Product transfer path (e.g. via the subscriber's OnBeforeTransferFieldData or a full sync call) and asserts the resulting CRM Product has ConvertToCustomerAsset = false. Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6 |
…ronization logic for service items
…procedure to streamline service item synchronization
…re and update test cases for service item synchronization
| CRMProduct.ConvertToCustomerAsset := false; | ||
| CRMProduct.Insert(false); | ||
| // [GIVEN] A coupled item and product where Convert to Customer Asset is Yes. | ||
| CRMSetupDefaults.ResetItemProductMapping('ITEM-PRODUCT', false); |
There was a problem hiding this comment.
The new Item-to-CRM Product sync test (ItemSynchronizationDisablesCustomerAssetConversion) only exercises an already-coupled, pre-existing product via CreateCoupledItemAndProduct, so it never covers the new DestinationIsInserted = true branch added in OnAfterTransferRecordFields (the conditional LoadFields(ConvertToCustomerAsset) path taken when a CRM Product is newly created from an item). Add a test case that synchronizes an uncoupled item so a new CRM Product is inserted, and assert the resulting ConvertToCustomerAsset is correctly initialized/disabled.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
…multi-company synchronization
| IntegrationFieldMapping.Direction::ToIntegrationTable, | ||
| '', false, false); | ||
|
|
||
| // Business Central service items are the source for Field Service customer assets. |
There was a problem hiding this comment.
The new 'Item-CRM Product' synchronization branch and its Item-Product field mapping constant (ConvertToCustomerAsset = 'false') change the intended behavior for existing tenants: previously-coupled items and their CRM Products keep their pre-30.0 field mapping and Convert-to-Customer-Asset state until a user manually runs 'Reset Configuration' or a full resync occurs. No upgrade code migrates already-installed Integration Field Mapping rows or already-coupled CRM Product records to the new 'service items always synchronize' design, so the fix does not apply safely to tenants that upgrade without manual intervention.
Knowledge:
- microsoft/knowledge/upgrade/upgrade-codeunit-subtype.md
- microsoft/knowledge/upgrade/install-code-does-not-run-on-version-upgrade.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
…ate_Field_Service_Customer_Asset_Created
…ation tests for item variant support
| end; | ||
|
|
||
| /// <summary> | ||
| /// Retained for compatibility. Service items are now always synchronized to Field Service customer assets, so this procedure leaves the synchronization decision unchanged. |
There was a problem hiding this comment.
The obsolete public test-library procedure IgnoreServiceItemsByConvertToCustomerAssetFlag (Microsoft.TestLibraries.DynamicsFieldService, FSIntegrationTestLibrary.Codeunit.al) is correctly marked [Obsolete('...', '30.0')], but its body was replaced with an empty no-op instead of continuing to forward to the previous implementation. During the deprecation window, existing external callers who still invoke this public procedure expecting IgnoreRecord to be set now silently receive unchanged behavior. Guidance on deprecating public members with the Obsolete lifecycle recommends preserving the prior behavior (e.g., keep forwarding to the codeunit's still-present implementation, or otherwise retain the previous decision) until the procedure is actually removed in a later release, rather than emptying it immediately upon obsoletion.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
| 'Item-CRM Product': | ||
| begin | ||
| DestinationRecordRef.SetTable(CRMProduct); | ||
| if DestinationIsInserted then |
There was a problem hiding this comment.
The new 'Item-CRM Product' case branch in OnAfterTransferRecordFields has a DestinationIsInserted path that calls CRMProduct.LoadFields(ConvertToCustomerAsset) before disabling customer-asset conversion, but the new tests (ItemSynchronizationDisablesCustomerAssetConversion, ItemProductMappingDisablesCustomerAssetConversion) only exercise the already-coupled/update path via CreateCoupledItemAndProduct. No test drives CRMIntegrationTableSynch.SynchRecord through a path where the CRM Product record is newly inserted, so the DestinationIsInserted = true branch (and its LoadFields call) is not covered by this change.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
…mer_Asset_Created
…tatus and streamline related procedures
…RMProduct record and adding new helper methods for item coupling checks
…mer_Asset_Created
…mer_Asset_Created
…S_Duplicate_Field_Service_Customer_Asset_Created
Jesper Schulz-Wedde (JesperSchulz)
added
Team: SCM
GitHub Actions
Active
github-actions
Bot
added
Ownership: Needs Review
GitHub Actions
Active
github-actions
Bot
added
Team: SCM
Good Sense Reviewer - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis change keeps Business Central as the source for Field Service customer assets for synchronized items. It removes the service-item filter that depended on the Product's Convert to Customer Asset flag, and it adds item-product synchronization logic that turns that flag off for coupled items. The main behavior looks correct for the reported duplicate-asset scenario. The service-item mapping depends on the item-product mapping, the new item-product transfer path uses the coupling state to derive the Product value, and the tests cover both the mapping setup and a coupled item/product synchronization. The relevant integration event publishers and call sites were checked, including the field-modified and transfer-field hooks used by this change. Problem-solution fitFit: Strong The reported problem is that both systems can create a customer asset for the same equipment. The change addresses the root cause by always allowing the Business Central service item to synchronize and by disabling native Field Service customer asset conversion on coupled Products. SuggestionsS1 (🟠 Moderate): Guard field-modified handling when disabled Risk assessment and necessityRisk: The main regression surface is item-to-Product synchronization and service-item-to-customer-asset synchronization for coupled Field Service data. The change is not financial posting logic and does not change schema, but it changes integration behavior for coupled Products and can affect customers that rely on native Field Service conversion for those Products. Necessity: The change is needed because the old Product flag could suppress Business Central customer-asset synchronization while still allowing Field Service to create another asset during work order completion. The scope is targeted: it keeps service items synchronizing and moves the conflict prevention to the item-product mapping where the Product flag is maintained.
|
…scriber and add corresponding test case
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThe latest change addresses the remaining field-comparison gap from round 1. The field-modified subscriber now uses the same Field Service enabled guard as the transfer subscriber, and the added test verifies that the comparison hook stays unhandled when integration is disabled. The current diff still fits the reported duplicate-customer-asset scenario. The item-product mapping derives Convert to Customer Asset from the item coupling state, service items are no longer skipped by that Product flag, and the comparison and transfer hooks now stay consistent when Field Service integration is off. Status of previous suggestions
New observations (commits since round 1)None - changes only addressed the previous suggestion. Risk assessment and necessityRisk: The regression surface remains item-to-Product synchronization and service-item-to-customer-asset synchronization for coupled Field Service data. The latest change lowers the disabled-integration risk by making field comparison and field transfer follow the same guard. Necessity: The change is still needed to prevent both systems from creating customer assets for the same equipment. The round-2 change is also necessary because it keeps the new mapping inactive when Field Service integration is disabled.
|
3 participants
What & why
Business Central and Field Service could both create a Customer Asset for the same item, resulting in duplicates when a Work Order was completed.
This change keeps Business Central as the source for Customer Asset creation by:
Linked work
Fixes AB#649195
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility
Risk is low and limited to coupled Field Service Products. Their Convert to Customer Asset value is overwritten to No during Item synchronization, preventing native Field Service asset creation.
Uncoupled Field Service Products are unaffected. No schema or public API changes are introduced. Existing configurations are covered by runtime enforcement, while reset or newly created mappings receive the constant outbound mapping.